Split routes by verb during compilation#6739
Conversation
|
I think it's fine to just mention it in the changelog. Alternatively, maybe we could detect such dead routes at compile time and drop them to keep the same behavior, emitting a deprecation warning? Then we drop that extra code for 2.0. |
This pull request compiles routes by verb, which speeds up
compilation times for large routes. This is done by making
sure `match/forward` statements always come last.
Therefore, this pull request changes the semantics of *dead
routes*. If you had this code:
match "/foo"
get "/foo"
The second route would never be matched but it is now as part
of this pull request. However, `get "/foo"` should not be there
in the first place. So the routes should either be rearranged
or removed anyway.
josevalim
force-pushed
the
jv-router-split
branch
from
a8bfabf to
a2abbbb
Compare
Co-authored-by: Steffen Deusch <steffen@deusch.me>
On Elixir 1.20 the set-theoretic type checker's per-module cost is super-linear in route count, and phoenixframework#6739's per-verb functions live in one module, so the type-check phase is unchanged on large routers. Emit each verb's match clauses (chunked) and the &Controller.action checks into sub-modules so each is verified in its own parallel module pass. The router keeps verb-keyed dispatch and resolves pipelines by index. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rhcarvalho
left a comment
rhcarvalho
left a comment
There was a problem hiding this comment.
It's wonderful how many optimization possibilities are open :)
... or put this change behind an option.
Considering this could cause unintended change in behavior during an upgrade and be hard to detect (warnings can be missed before and after pushing to production, we may miss some corner case), I think an opt-in approach would be safer for existing production apps.
On Elixir 1.20 the set-theoretic type checker's per-module cost is super-linear in route count, and phoenixframework#6739's per-verb functions live in one module, so the type-check phase is unchanged on large routers. Emit the match clauses (chunked) and the &Controller.action checks into sub-modules so each is verified in its own parallel module pass. Dispatch into the sub-modules is kept dynamic on purpose: a static call would make the type checker re-aggregate every route's return type into the router module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@rhcarvalho @SteffenDE it should be good now. We always warn if match :* and forward are not last and then we only optimize in case they are. |
|
@josevalim I found at least one gap related to the interaction with test "does not warn and remains optimized on disjoint scopes" do
# This test demonstrates the gap: scope /admin and scope /api do not overlap.
# Defining forward in /admin before get in /api should not warn, but currently it does.
warnings = ExUnit.CaptureIO.capture_io(:stderr, fn ->
defmodule DisjointScopesRouter do
use Phoenix.Router
scope "/admin" do
forward "/", UserController
end
scope "/api" do
get "/users", UserController, :index
end
end
end)
assert warnings == ""
end |
|
I'm thinking we should move this behind a compile-time option. Then projects with very large routers can opt in, but by default, the existing behavior is kept. |
|
@rhcarvalho I don't think it is practical for us to detect all of those cases, that's why the suggestion is to move all forward/match-star to the end of the file. I'd ideally prefer to not have two compilation mechanisms in the long term. Are we against asking to move all of them to the end? |
|
The issue I see with asking people to move them is that it means that people effectively need to duplicate scopes that define such routes, just to satisfy the optimization, even if that optimization does not bring a measurable performance improvement to a regular-sized router. scope "/admin" do
pipe_through [:browser, :require_admin]
get "/foo", ...
post "/bar", ...
forward "/dashboard", Dashboard
end
scope "/api" do
pipe_through [:api, :require_api_token]
get "/baz", ...
match :*, "/other", SomeController, :any
endhas to become scope "/admin" do
pipe_through [:browser, :require_admin]
get "/foo", ...
post "/bar", ...
end
scope "/api" do
pipe_through [:api, :require_api_token]
get "/baz", ...
end
scope "/admin" do
pipe_through [:browser, :require_admin]
forward "/dashboard", Dashboard
end
scope "/api" do
pipe_through [:api, :require_api_token]
match :*, "/other", SomeController, :any
endand suddenly you have to remember to keep the pipelines in sync... |
|
I made it opt-in now. Now in theory we can backport this to the v1.8 branch... |
rhcarvalho
left a comment
rhcarvalho
left a comment
There was a problem hiding this comment.
I agree with Steffen's observation re: forcing a scope to be split and pipelines kept in sync, but considering the opt-in approach it sounds like a great option for those needing the added performance.
@josevalim yes, a single compilation strategy would be best, but this seems too risky to force onto everyone else by default.
|
💚 💙 💜 💛 ❤️ |
3 participants
This pull request groups compiled routes by verb, which speeds up compilation times for large routers up to 3x. A side-effect is that
match/forwardroutes are always matched last.Therefore, this pull request changes the semantics of dead routes. If you had this code:
The second route would never be matched but it is now as part of this pull request. However,
get "/foo"should not be there in the first place. It is effectively a dead route.Currently WIP. We should either document this pitfall or put this change behind an option. I am fine with documenting this in the CHANGELOG as the code above should either not exist or be correct anyway.